-
-
Notifications
You must be signed in to change notification settings - Fork 435
Fix TmpDir to match POSIX #1462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Way nicer, slight change needed for backwards compatibiity (sorry) 🙏
src/pyinfra/facts/server.py
Outdated
elif [ -n "$TEMP" ] && [ -d "$TEMP" ] && [ -w "$TEMP" ]; then | ||
echo "$TEMP" | ||
else | ||
echo "/tmp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't provide a default here, the fallback is specified at the config level currently (https://github.com/pyinfra-dev/pyinfra/blob/3.x/src/pyinfra/api/config.py#L29).
To be honest I think this way is nicer, but we can't break the config for backwards compatibility so if no matching env vars the fact still needs to return a blank string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the TMPDIR|TMP|TEMP
are set in my arch/zsh terminal or in another ubuntu/bash PC I checked.
I had the this issue and the initial problem was server.script
not working. The underlying issue was that this fact returning empty. I remember seeing something about setting a default in the pyinfra code, but did not check in details. Still the script
operation was failing (v3.5
).
So if this returns a blank, my guess is it still would fail here in my machine, unless the script
was fixed in the meantime to use the proper default (if that is how it should work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed the change, let me know @Fizzadar
Fix #1458
According to POSIX standards, checks environment variables in this order:
1. TMPDIR (if set and accessible)
2. TMP (if set and accessible)
3. TEMP (if set and accessible)
4. Falls back to /tmp
3.x
at this time)scripts/dev-test.sh
)scripts/dev-lint.sh
)